Bound catastrophic regex backtracking in user-authored patterns - #5037
builder-io-integration[bot] wants to merge 9 commits into
Conversation
…he browser An agent asked to make "Full Name" accept at least two words wrote `^([A-Za-z]+\s?)+$`. It compiles cleanly and backtracks exponentially: 748 ms on a 26-character value, doubling with every further character. The Forms editor tab stopped responding and Chrome offered to kill the page. The same pattern is re-checked in the submit handler, so it pegs the server event loop too, and no JavaScript timeout can interrupt a match once V8 is inside it. Add analyzeRegexSource / compileUserRegex / testUserRegex to @agent-native/core/shared, which refuse patterns shaped like this rather than trying to time them out, and return a tri-state so "was not evaluated" stays distinguishable from "did not match". Forms rejects an unsafe pattern at the authoring gate shared by create-form, update-form and patch-form-fields, naming a safe rewrite so the agent can correct itself; the fill page, submit handler and public SSR runtime bound patterns already stored. The same defect was present in Calendar booking fields, whose existing input/pattern length caps are ineffective against it, and in the Slides regex-replace edit op. Also teach guard:i18n-changed-copy the export-default messagesByLocale wrapper shape, which apps using it could not otherwise satisfy.
|
@builderio-bot look at the latest PR feedback and fix anything you agree with. Be skeptical. Reply in each open inline thread with exactly one of:
Or resolve the thread in GitHub. Outdated threads after new commits do not need a new reply. Get CI green and keep the branch mergeable. |
…77ac63c899f04da0a544
Six false negatives, each runtime-validated as catastrophic before fixing:
- The probe alphabet was a fixed list, so any pattern over characters it omitted
produced empty character sets and read as unambiguous. `^(A+)+$` and `^(x|x)+$`
were cleared; `^(a+)+$` only failed because "a" happened to be in the list.
The alphabet is now derived from the pattern itself, and an atom the probes
cannot describe is "unknown" and overlaps everything rather than being treated
as disjoint.
- Flags are now part of the verdict. `^(a|A)+$` is unambiguous on its own and
catastrophic under `i`; the Slides regex-replace op analyzed the source while
running it with flags.
- Ambiguity checks keyed on infinite quantifiers only, so `^(a{1,10})+$` evaded
them. They now key on variable length.
- Overlapping alternatives were only compared when both sides were a single
atom, clearing `^(a|aa)+$`. They are now compared on leading characters and
minimum length, which still admits `(cat|car)+`.
- Three chained repetitions over the same characters backtrack cubically and
exceed the input cap: `^(a+)(a+)(a+)$` takes over 20s at 4096 characters. Runs
of three or more are now rejected; pairs are quadratic and stay admitted, so
the standard email pattern is unaffected.
- The public form runtime skipped the pattern check for values over the cap, so
an unchecked value looked valid in the browser while the server rejected it.
It now reports the value as unchecked.
Corpus expanded to 16 catastrophic and 17 legitimate patterns; every cleared
pattern verified at 0 ms against adversarial input at the full 4096-char cap.
There was a problem hiding this comment.
Builder reviewed your changes and found 6 potential issues 🔴
Review Details
Incremental Code Review Summary
The latest commit materially addresses the six previously reported issues: the analyzer now collects pattern literals, fails closed on unknown character sets, recognizes variable-length alternatives and chained repetitions, and receives Slides flags; public SSR now reports over-limit values instead of silently skipping them. Those prior review threads were verified fixed and resolved.
New review passes still find remaining ReDoS bypasses in the expanded heuristic. This remains high risk because these patterns can execute synchronously in Forms/Calendar validation or over unrestricted Slides content.
New Findings
- 🔴 HIGH: Finite outer repetition groups are skipped, allowing patterns such as
^(a+){10}$to reach synchronous evaluation. - 🔴 HIGH: Duplicate multi-character alternatives can still create exponential ambiguity.
- 🔴 HIGH: Slides dotAll (
s) semantics are not preserved during probing. - 🔴 HIGH: Unicode case folding and Unicode property escapes can disagree with the actual runtime regex when
uis used. - 🟡 MEDIUM: Public SSR rejects optional empty fields with unsafe patterns even though the client and server submission paths skip validation for absent values.
The new regression corpus and focused tests are valuable, and the core tri-state contract remains sound. The remaining bypasses should be closed before merge.
🧪 Browser testing: Will run after this review (PR touches UI code)
| for (const atom of branch) { | ||
| if (atom.kind !== "group") continue; | ||
| if (isUnbounded(atom)) { |
There was a problem hiding this comment.
🔴 Analyze finite outer repeated groups before executing them
walk only sends a group to analyzeRepeatedGroup when its outer quantifier is unbounded. A finite repeat can still have a combinatorial number of partitions within the 4,096-character input cap; for example, ^(a+){10}$ is currently classified safe and can take seconds on a short non-matching input. Analyze repeated groups with finite bounds when their bodies are variable or ambiguous, or fail closed for these cases.
Additional Info
Found by 2/3 review agents; independently runtime-validated.
There was a problem hiding this comment.
Required — fixed: confirmed. ^(a+){10}$ did not return in 12s on a 41-character non-match, and the analyzer cleared it — walk only called analyzeRepeatedGroup when the outer quantifier was Number.POSITIVE_INFINITY. It now runs on any group with max > 1, since a finite bound does not remove the fan-out across iterations. isUnbounded had no other caller and is gone.
Widening this immediately produced a false positive worth recording: ^[A-Z]{2}\d{2}[A-Z0-9]{4}\d{7}([A-Z0-9]?){0,16}$ — a textbook IBAN pattern — started failing the junction rule, and measures 0ms. A body that can consume at most one character has nothing to hand back and forth between iterations, so analyzeRepeatedGroup now skips branches whose maxIterationLength is under two. That also clears the pre-existing ^(a?)+$ false positive. Both are in the linear corpus now.
| const ambiguous = | ||
| minLength(a) !== minLength(b) || | ||
| a.some(isVariableLength) || | ||
| b.some(isVariableLength) || | ||
| (consuming(a).length === 1 && consuming(b).length === 1); |
There was a problem hiding this comment.
🔴 Reject duplicate multi-character alternatives in repeated groups
The ambiguity check permits equal-length fixed alternatives unless both branches are single atoms. This misses duplicate multi-character branches such as ^(ab|ab)+$, which provide indistinguishable choices on every iteration and can backtrack exponentially on a non-match. Detect equal-language or overlapping multi-atom alternatives conservatively before execution.
Additional Info
Found by 1/3 review agents; independently runtime-validated.
There was a problem hiding this comment.
Required — fixed: confirmed exponential — ^(ab|ab)+$ on an ab-repeat non-match goes 33 chars/9ms, 41/19ms, 45/79ms, 49/309ms, doubling every two characters. The alternative check cleared equal-minLength fixed branches unless both were single atoms, which is exactly the (a|a)+ special case and nothing wider.
Added sameLanguage: two branches with the same number of consuming atoms, matching min/max per position and equal probe character sets per position, are an indistinguishable choice on every iteration. Two atoms the probe alphabet cannot describe count as not-provably-different, so it stays fail-closed. ^(ab|ab)+$ and ^(abc|abc|x)+$ are in the catastrophic corpus; ^(cat|car)+$, ^(ab|ac)+$ and ^(GET|PUT)$ differ in some position and stay in the linear one.
| // Only case folding changes which characters two atoms share; the rest affect | ||
| // anchoring or iteration, and `y`/`g` would break the single-character probes. | ||
| const probeFlags = flags.includes("i") ? "i" : ""; |
There was a problem hiding this comment.
🔴 Preserve dotAll semantics during overlap analysis
Only i is retained in probeFlags, but Slides passes s through to the actual RegExp and matchAll. With dotAll, . can overlap newline-consuming alternatives that the analyzer treats as disjoint, allowing a catastrophic pattern to block the Slides request. Preserve matching-affecting flags such as s, or fail closed for unsupported flag combinations.
Additional Info
Found by 2/3 review agents; independently runtime-validated.
There was a problem hiding this comment.
Required — fixed: confirmed. ^(.|\n)+Z$ against a run of newlines measures 0ms without flags and 169ms/450ms/7.2s at 20/24/28 characters under s — dotAll makes . and \n overlap, which is precisely what the analyzer was told to ignore.
probeFlags now keeps every flag that changes what a character matches: i, s, u, v. m and d cannot affect a single-character membership test, and g/y are still dropped because lastIndex would carry between probes and corrupt the sets. Covered by a test asserting opposite verdicts for the same source with and without s.
| } | ||
| // Only case folding changes which characters two atoms share; the rest affect | ||
| // anchoring or iteration, and `y`/`g` would break the single-character probes. | ||
| const probeFlags = flags.includes("i") ? "i" : ""; |
There was a problem hiding this comment.
🔴 Preserve Unicode case-folding semantics during analysis
The analyzer drops u from probe flags even when the executed Slides regex uses iu. Unicode case folding can make alternatives overlap only under those flags (for example ^(ſ|s)+Z$ with iu), so the source can pass analysis while the runtime backtracks catastrophically. Preserve u with i or reject flag combinations whose matching semantics are not modeled.
Additional Info
Found by 1/3 review agents; independently runtime-validated.
There was a problem hiding this comment.
Required — fixed: the example holds exactly as described. ^(ſ|s)+Z$ against 25 characters measures 0ms under i and 2194ms under iu — Unicode case folding is what makes ſ and s the same character, and dropping u from the probes hid it.
Fixed with the same change as the dotAll thread: u and v are now preserved alongside i and s. Retaining u also means a probe like ^(?:\p{L})$ compiles at all; when it cannot, charSetOf already returns unknown, which overlaps everything. Test asserts iu unsafe and i safe for this source.
| } else if (ch === "\\") { | ||
| source = state.source.slice(state.index, state.index + 2); | ||
| state.index += 2; | ||
| kind = /^\\\d$/.test(source) ? "backref" : "escape"; |
There was a problem hiding this comment.
🔴 Parse or reject Unicode property escapes
The parser treats every escape as two characters, so under u, \p{L} is parsed as \p followed by literal {L} rather than a Unicode property class. A pattern such as ^(\p{L}+)+$ can therefore be classified safe while the actual JavaScript regex has nested-overlap backtracking. Parse multi-character property escapes correctly or fail closed whenever unsupported escape syntax is present.
Additional Info
Found by 1/3 review agents; independently runtime-validated.
There was a problem hiding this comment.
Required — fixed, with one correction to the finding: the parse defect is real, but ^(\p{L}+)+$ was already rejected. It was reaching the adjacent-repetition rule with the reason "adjacent repetitions (L and }+) that match the same characters" — the stray } picked up the quantifier, so the verdict described a pattern nobody wrote, and the rejection was accidental rather than wrong.
Accidental is not a property to rely on, and misparsing cuts both ways, so \p{...} and \P{...} now parse as a single class atom; an unterminated one bails, which fails closed. With u preserved in the probes (the sibling thread) the atom is probed correctly instead of as literal p. Tests: ^\p{L}+$ and ^\p{L}+ \p{L}+$ clear, ^(\p{L}+)+$ is rejected with \p{L} named in the reason.
| return (f.validation.message || f.label + " must be at most " + f.validation.max); | ||
| if (f.validation.pattern && typeof v === "string" && !new RegExp(f.validation.pattern).test(v)) | ||
| return (f.validation.message || f.label + " is invalid"); | ||
| if (f.validation.unsafePattern) |
There was a problem hiding this comment.
🟡 Do not reject optional empty fields solely for an unsafe pattern
The inline SSR runtime checks unsafePattern before determining whether the field has a submitted value. For an optional untouched text field, the SSR page therefore returns an uncheckable-pattern error, while the React client and server submission validator skip pattern checks for absent values. Move this branch inside the present-string validation path or make all surfaces consistently handle optional empty fields.
Additional Info
Found by 2/3 review agents; independently validated against the existing client/server behavior.
There was a problem hiding this comment.
Required — fixed: the inconsistency is real. validateSubmissionField returns early on isAbsentSubmissionValue, so the submit handler never pattern-checks a blank field, while the SSR runtime read unsafePattern before looking at the value and failed an optional field the respondent never touched.
The runtime now computes hasValue first and skips both the unsafePattern message and the pattern test when the value is absent. I also applied the same rule to FormFillPage, which tested patterns against "" — that one predates this PR, but it means the React page rejects submissions the server accepts, and for an unsafe stored pattern this PR would have made it reject them with an uncheckable-rule message. All three surfaces now agree that an empty value is the required check's business. Covered in validation-pattern-redos.spec.ts.
|
There was a problem with your request, please try again later. Error id: |
`pnpm fmt:check` fails on main for this changelog file, which blocks every PR that merges it. Whitespace only.
…da0a544 # Conflicts: # scripts/guard-i18n-changed-copy.test.ts # scripts/guard-i18n-changed-copy.ts
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest head still contains the six previously reported issues, which remain open and were intentionally not reposted. New review found two medium-severity regressions. Slides now analyzes agent-supplied regex-replace patterns directly, bypassing the shared 512-character source limit; the analyzer itself can therefore become expensive before slide matching begins. Separately, applying the new safety verdict inside assertValidFields also affects legacy persisted Forms rows during submission/upload configuration parsing, so old forms with unsafe patterns can fail with a generic 500 before the intended unevaluated response is reached.
The core approach and focused regression suites remain useful, but these new integration boundaries should be fixed before merge. A low-severity test-helper concern was also identified: the synchronous elapsed-time helper cannot interrupt a truly hanging regex; it is included for tracking but may be filtered by the review service.
Risk remains high because these paths process agent- or user-authored expressions synchronously.
🧪 Browser testing: Will run after this review (PR touches UI code)
| // Name the mistake so the agent rewrites the pattern instead of retrying it. | ||
| // The flags are part of the verdict: `^(a|A)+$` is unambiguous on its own and | ||
| // catastrophic under `i`. | ||
| const verdict = analyzeRegexSource(edit.pattern, flags); |
There was a problem hiding this comment.
🟡 Bound regex-replace source before running the analyzer
regex-replace accepts an unbounded pattern and calls analyzeRegexSource directly, bypassing the 512-character limit enforced by compileUserRegex. A large syntactically valid pattern can make probe collection and adjacent-run analysis quadratic before slide content is even inspected, stalling the Slides request. Enforce the shared source bound before analysis, or reuse a bounded compiler while preserving the requested flags.
Additional Info
Found by 1/2 review agents; independently measured on the current implementation.
There was a problem hiding this comment.
Required — fixed, at the primitive rather than at the call site. Measured on the previous implementation with ^(a1|a2|…)+$: 50 branches/15ms, 100/42ms, 200/280ms, 400/2.0s, 800/15.6s. The pair-wise alternative comparison is the cubic part, and regex-replace was the one caller reaching analyzeRegexSource without compileUserRegex's cap in front of it.
Capping only Slides would leave the next direct caller to rediscover this, so analyzeRegexSource now enforces MAX_USER_REGEX_LENGTH itself and returns an unsafe verdict naming the length. compileUserRegex still short-circuits first, so Forms and Calendar behaviour is unchanged. I also memoized charSetOf per atom, which is where the repeated RegExp compiles were: 304 characters went from 42ms to 13ms. Slides test asserts the over-long pattern is refused in under a second.
|
@builderio-bot look at the latest PR feedback and fix anything you agree with. Be skeptical. Reply in each open inline thread with exactly one of:
Or resolve the thread in GitHub. Outdated threads after new commits do not need a new reply. Get CI green and keep the branch mergeable. |
…77ac63c899f04da0a544
|
There was a problem with your request, please try again later. Error id: |
- analyze any group that can iterate more than once, not only unbounded
ones: ^(a+){10}$ never returns on a 41-character non-match
- flag duplicate alternatives inside a repeated group (^(ab|ab)+$)
- carry s/u/v into the character-set probes alongside i, so ^(s|s)+Z$
under iu and ^(.|\n)+Z$ under s are no longer analyzed away
- parse \p{...} as one atom instead of an escape plus a literal
- cap and memoize the analysis itself; it is super-linear in the pattern
length and Slides reached it without compileUserRegex's cap
- exclude lookarounds from consuming-atom rules and skip bodies that can
only take one character, which were rejecting the standard password
rule and an IBAN pattern
Scope the Forms authoring gate to write paths: the submit and upload
handlers re-validate stored fields, where refusing the configuration
replaces the field-level reason with a generic 500. Skip pattern checks
for absent values in the SSR runtime and the React fill page, matching
what the submit handler has always done.
Forty mdx files arrived unformatted with the latest main merge and fail fmt:check. Column alignment only.
There was a problem hiding this comment.
Builder reviewed your changes and has a few items to flag 🟡
Review Details
Incremental Code Review Summary
The latest commit adds the patternSafety: false option for legacy Forms read paths and wires it into both submission and upload handlers. This correctly preserves structural validation while allowing stored unsafe patterns to reach testUserRegex and produce a controlled field-level response; the prior legacy parsing comment was verified fixed and resolved. Focused bounded-regex, Forms, Calendar, and Slides suites pass with 97 tests.
The previously open regex/analyzer and SSR issues remain unresolved and were intentionally not reposted. Two new low-severity documentation rendering regressions were found in Korean localized docs: formatter changes split an inline CLI command and remove continuation indentation from a list item. These are non-blocking documentation issues and may be filtered by the review service.
Risk remains high because the unresolved regex-analysis findings still affect synchronous user-authored expression execution.
🧪 Browser testing: Will run after this review (PR touches UI code)
|
DO NOT MERGE |
Summary
Adds a shared
bounded-regexutility that detects and refuses regex patterns shaped for catastrophic (super-linear) backtracking, and wires it into every place Forms/Calendar/Slides evaluates a user- or agent-authored regex, fixing a tab-freezing hang.Factory item:
49878fbab5aae6e665ddf4f2ed8cf959d016fe863d9a039a3f088085da7e5353Source Slack thread: https://slack.com/app_redirect?team=T0GCV21GE&channel=C0ATH3CCZT4&message_ts=1789450650.422179
Problem
In the Forms editor, an agent asked to make "Full Name" require at least two words wrote the pattern
^([A-Za-z]+\s?)+$. That pattern is short, compiles cleanly, but backtracks exponentially: a 26-character non-matching value already costs ~750ms, doubling with every additional character. Because the same pattern was applied vianew RegExp(source).test(value)with no timeout mechanism available in JavaScript, this froze the Forms editor tab (CPU spike, Chrome "Page Unresponsive" dialog) and, since the same pattern was re-checked on submit, could also peg the server's event loop. Existing mitigations (capping pattern length to 200 chars, capping input to 1000 chars) did not help — the blowup is reached well inside those caps.Solution
Introduced
analyzeRegexSource,compileUserRegex, andtestUserRegexin@agent-native/core/shared.analyzeRegexSourceparses the pattern's structure and recognizes known ambiguity signatures that cause super-linear backtracking (nested/adjacent overlapping repetition, nullable parts under an unbounded repeat, overlapping single-atom alternatives) and refuses to run those patterns. Safe patterns are still evaluated against a capped input length. Results are tri-state (match/no-match/unevaluated) so a refused-to-run pattern can never be silently read as "validation passed" or "value failed."Key Changes
packages/core/src/shared/bounded-regex.ts: exportsanalyzeRegexSource,compileUserRegex,testUserRegex,MAX_USER_REGEX_LENGTH,MAX_USER_REGEX_INPUT_LENGTH, plus a hand-rolled regex-shape parser/analyzer and spec coverage (bounded-regex.spec.ts).templates/forms/shared/field-schema.ts,templates/forms/server/lib/validate-fields.ts):assertValidFieldsnow rejects unsafe patterns at save time viacompileUserRegex, with an error message suggesting a safe rewrite (e.g.^\S+(\s+\S+)+$for "at least two words").templates/forms/server/lib/submission-validation.ts,templates/forms/app/pages/FormFillPage.tsx): usetestUserRegexinstead of rawnew RegExp(...).test(...), handling theunevaluatedcase explicitly rather than silently accepting/rejecting.templates/forms/server/lib/public-form-ssr.ts): newpublicValidationhelper strips an unsafepatternbefore shipping field validation to the anonymous respondent's browser, replacing it with anunsafePatternmarker so the inline runtime can show a "can't be checked" message instead of running the pattern.templates/calendar/app/components/booking/BookingForm.tsx,templates/calendar/server/handlers/bookings.ts): replaced ad-hoc length caps and try/catchRegExpusage withtestUserRegex, distinguishing "no match" from "uncheckable" on both client and server.templates/slides/server/lib/slide-content-patch.ts):applyRegexReplacenow callsanalyzeRegexSourcebefore runningmatchAll, throwing a descriptive error so the agent rewrites the pattern instead of hanging on it.fieldPatternUncheckable(Calendar) anduncheckablePattern(Forms) strings across all locales.templates/calendar/server/lib/booking-custom-field-pattern.spec.tsandtemplates/forms/server/lib/validation-pattern-redos.spec.ts, both time-bounded so a reintroduced catastrophic pattern fails fast instead of hanging the suite.scripts/guard-i18n-changed-copy.tsnow also recognizes theexport default messagesByLocale[...]wrapper shape (used by calendar/brain templates) as forwarding an inline locale update, with accompanying tests.bounded-user-regex.mddocumenting the new@agent-native/coreshared utilities.To clone this PR locally use the Github CLI with command
gh pr checkout 5037You can tag me at @BuilderIO for anything you want me to fix or change